fix: CLI crash + auto-generate YAML config templates from schema#237
fix: CLI crash + auto-generate YAML config templates from schema#237
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
Fixes a cyclopts CLI parsing crash triggered when --load-pattern is combined with load-pattern subfields like --target-qps / --concurrency in the online benchmark command.
Changes:
- Annotates
LoadPatternto adjust how cyclopts maps nested parameters (@cyclopts.Parameter(name="*")). - Updates the CLI parameter definition for
LoadPattern.typeto avoid the prior name collision.
Comments suppressed due to low confidence (1)
src/inference_endpoint/config/schema.py:360
- This change is a regression fix for a CLI crash when combining
--load-patternwith nested load-pattern fields (e.g.--target-qps). There’s existing automated test coverage for config validation intests/unit/commands/test_benchmark.py, but no test currently exercises cyclopts parsing for this flag combination.
Add a regression test that parses benchmark online ... --load-pattern poisson --target-qps 100 (or directly parses OnlineBenchmarkConfig via cyclopts) and asserts it no longer raises and that config.settings.load_pattern.type/target_qps are set as expected.
@cyclopts.Parameter(name="*")
class LoadPattern(BaseModel):
"""Load pattern configuration.
Different patterns use target_qps differently:
- max_throughput: target_qps used for calculating total queries (offline, optional with default)
- poisson: target_qps sets scheduler rate (online, required - validated)
- concurrency: issue at fixed target_concurrency (online, required - validated)
"""
model_config = ConfigDict(extra="forbid", frozen=True)
type: Annotated[
LoadPatternType,
cyclopts.Parameter(name="--load-pattern", help="Load pattern type"),
] = LoadPatternType.MAX_THROUGHPUT
target_qps: Annotated[
float | None, cyclopts.Parameter(alias="--target-qps", help="Target QPS")
] = Field(None, gt=0)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request modifies the LoadPattern class in the configuration schema by applying a class-level cyclopts.Parameter decorator and updating the type field's parameter definition to use the name argument instead of alias. I have no feedback to provide.
arekay-nv
left a comment
There was a problem hiding this comment.
Can we also add a test for this - seems like a change that shouldn't have gone in.
90fe9c8 to
80a79ef
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review Council — Multi-AI Code ReviewReviewed by: Claude (Codex ran but produced investigation output, not structured findings) | Depth: standard Found 3 issues across 3 files:
Also addressed all Copilot review comments (pinned SHAs, quoted pip install, heredoc for inline Python, expanded pre-commit |
8915750 to
ffb87d9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/inference_endpoint/config/templates/concurrency_template.yaml
Outdated
Show resolved
Hide resolved
ffb87d9 to
b781ff7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/inference_endpoint/config/templates/online_template_full.yaml
Outdated
Show resolved
Hide resolved
src/inference_endpoint/config/templates/offline_template_full.yaml
Outdated
Show resolved
Hide resolved
src/inference_endpoint/config/templates/concurrency_template_full.yaml
Outdated
Show resolved
Hide resolved
4b2c8e7 to
93b8808
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
93b8808 to
e916b74
Compare
e916b74 to
ea428c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c4a1c6b to
302f591
Compare
302f591 to
f030fd4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bug fix: - CLI crash on --load-pattern + --target-qps (IndexError) — LoadPattern.type used alias= instead of name= on cyclopts.Parameter Template generation: - regenerate_templates.py generates minimal + full YAML templates from Pydantic schema field defaults (no hardcoded values) - Full templates include inline # comments auto-discovered from Field(description=) and Enum/Literal annotations - Minimal templates: required fields + placeholders only - Pre-commit hook auto-regenerates on schema changes; CI validates via CI env var detection (check-only mode) - init command uses model_dump(exclude_none=True) for offline/online/ concurrency; falls back to handwritten templates for eval/submission Schema improvements: - Added Field(description=) to all HTTPClientConfig, AccuracyConfig fields - New ScorerMethod enum (pass_at_1, string_match, rouge, code_bench_scorer, shopify_category_f1) for AccuracyConfig.eval_method Tests: - Hypothesis fuzz: 4000 random CLI flag combinations - Unit: all templates validate via from_yaml_file (auto-discovered glob) - Integration: generated templates run e2e against echo server - Init command: all 5 types tested Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f030fd4 to
6bd97e2
Compare
Summary
--load-pattern+--target-qps(IndexError) —LoadPattern.typeusedalias=instead ofname=oncyclopts.Parameterregenerate_templates.pyauto-generates minimal + full YAML templates from Pydantic schema defaults--checkmodeTemplates
Two variants per mode (offline, online, concurrency):
_template.yaml— minimal: only required fields +<PLACEHOLDER eg: example>values_template_full.yaml— all fields with schema defaults, inline# options:comments auto-discovered from Enum/Literal annotations, 2 dataset examples (perf + accuracy with parser columns), multiple endpointsInit command
inference-endpoint init offlinealways generates the full template. Available types:offline,online,concurrency.Tests
from_yaml_file(auto-discovered with glob)<>placeholders stripped to real values, and run end-to-end against the echo server — verifying templates produce functional configsDocs updated
Test plan
python scripts/regenerate_templates.py --checkpassespytest tests/unit/— 484 passedpytest tests/integration/commands/test_benchmark_command.py::TestTemplateIntegration— 6 templates run e2e🤖 Generated with Claude Code